Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use parameter level decorators for openapi params #940

Merged
merged 2 commits into from
Feb 5, 2018

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Jan 31, 2018

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Related API Documentation was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in packages/example-* were updated

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A mild -1 from me.

@@ -12,8 +12,10 @@ export class TodoController {
@repository(TodoRepository.name) protected todoRepo: TodoRepository,
) {}
@post('/todo')
@param.body('todo', TodoSchema)
async createTodo(todo: Todo) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing parameter-level decorators at method level was an intentional decision we made early on, so that we can support the following pattern:

class MyController() {
  @param.body('todo', TodoSchema) {
  async createTodo(...args) {
   return await this.todoRepo.create(...args);
  }   
}

As long as the example above keeps working (we keep it covered by our automated tests), I think the changes you are proposing here are a matter of subjective preference and can be landed if we all agree that's the style we want to promote.

I personally like more the old (method-level) style:

  • All REST decorators are specified in a single place (before the method)
  • The method signature is easier to read, because it's not cluttered by decorators

Compare:

// now
@get('/todo/{id}')
@param.path.number('id')
@param.query.boolean('items')
async findTodoById(id: number, items?: boolean): Promise<Todo> {
  // ...
}

// proposed
@get('/todo/{id}')
async findTodoById(
  @param.path.number('id') id: number, 
  @param.query.boolean('items') items?: boolean,
): Promise<Todo> {
  // ...
}

Having said that, the difference is not super important to me, so if there are more people in favor of your proposed style, then I am ok with that.

@strongloop/sq-lb-apex @strongloop/loopback-next opinions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos I think the method level decorators promote a top-down approach since the intended use for them seems to be is to have the users provide a schema, but I'm not so sure anymore. Currently, type inference is only possible with the parameter-level decorators and it seems like that's what we should promote since it is better for the UX, but I think it's also possible to infer the parameter types through method-level decorators, so I guess the change isn't strictly needed if we plan on adding in support for method-level decorators' parameter type inferrence.

Copy link
Contributor

@jannyHou jannyHou Feb 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: the following old comment is not a concern now. The new decorator requestBody allows overriding the generated schema by decorator's input.


@bajtos Interesting, I also come across a decision making about "method decorator" or "parameter decorator", or both.

The benefit of "parameter decorator" is it guarantees the consistency between openapi spec and controller function's inputs. But it may also have limitation:
Take the openapi3 requestBody spec as an example, one requestBody can have multiple content-type, if they don't share a common schema, we need to apply @param.body, which will equivalent to @requestBody in openapi3, to multiple parameters, but ideally one function only has one input for body data.

I started a discussion in #753 (comment)
And post my concern in #753 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I love the parameter decorator, even given my concern above, I would still like to try adjust the decorator code to support the multiple contents.

@raymondfeng
Copy link
Contributor Author

raymondfeng commented Feb 1, 2018

@bajtos IMO, use method level decorators for parameter metadata is NOT good practice as illustrated below.

  1. Sparse decoration of parameters can be achieved with method level @param.
myMethod(@param('p1') arg1, arg2, @param('p3') arg3) {}
  1. The usage can be confusing/misleading as we have to guess if (p1, p2) is for (arg1, arg2) or (arg2, arg3). Please note method decorators are applied from bottom to top, i.e., p2 -> p1.
@param('p1')
@param('p2')
myMethod(arg1, arg2, arg3) {}
  1. Method decorator does not provide index and implementation of the decorator needs to calculate number of params to help.

With your example:

class MyController() {
  @param.body('todo', TodoSchema) {
  async createTodo(...args) {
   return await this.todoRepo.create(...args);
  }   
}

Do you intend to say todo is the first argument? If so, it should be rewritten as:

class MyController() {
  async createTodo(@param.body('todo') todo: TodoSchema, ...otherArgs) {
   return await this.todoRepo.create(todo, ...otherArgs);
  }   
}

Or even:

class MyController() {
  async createTodo(@param.body('todo', TodoSchema) ...args) {
   return await this.todoRepo.create(...args);
  }   
}

BTW, using rest parameters in controller method is not a good idea anyway.

If you agree, I think we should deprecate or remove method level @param to avoid further confusion and damage.

@bajtos
Copy link
Member

bajtos commented Feb 2, 2018

Thank you all for thoughtful comments, you have convinced me that we should prefer argument-level param decorators over method-level style 👍

BTW, using rest parameters in controller method is not a good idea anyway.
If you agree, I think we should deprecate or remove method level @param to avoid further confusion and damage.

Let's open a new issue where we can discuss what to do about method-level @param and also determine what needs to be changed as a result (code, example projects, docs, etc.)

If possible, I'd like to get @ritch to comment on this change too, because it was him who was pushing hard for method-level param decorators IIRC.

@bajtos
Copy link
Member

bajtos commented Feb 2, 2018

coverage/coveralls — Coverage decreased (-0.01%) to 96.59%

@raymondfeng could you please check why our coverage went down? I was not able to spot the reason after a quick look. Are we perhaps missing tests verifying method-level param decorators, and now that you have removed method-level usage of those decorators, then the relevant code path is no longer covered by any tests? I would be surprised if that was the case though, I vaguely remember taking extra care of method-level usage when writing those @param decorators.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please consider my comment above about code coverage.

@raymondfeng
Copy link
Contributor Author

Opened #965 to follow up the removal/deprecation of method level @param.

@raymondfeng raymondfeng merged commit de969e4 into master Feb 5, 2018
@raymondfeng raymondfeng deleted the tidyup-getting-started branch February 5, 2018 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants